Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Add test to assert unique() behaviour #233

Merged
merged 1 commit into from
Dec 26, 2020

Conversation

localheinz
Copy link
Member

@localheinz localheinz commented Dec 22, 2020

This PR

  • adds tests to assert the behaviour of Generator::unique()

Related to #155.

πŸ’β€β™‚οΈ As a user of fakerphp/faker, I would like to continue to be able to use

$faker->unique()->word();

to generate a unique word.

I don't care if it internally uses extensions or providers, but I would like to continue using this behaviour. The tests added here ensure that this is the case.

@localheinz localheinz added the bug Something isn't working label Dec 22, 2020
@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #233 (53d6fce) into main (f53f67b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #233   +/-   ##
=========================================
  Coverage     58.40%   58.40%           
  Complexity     2044     2044           
=========================================
  Files           315      315           
  Lines          5104     5104           
=========================================
  Hits           2981     2981           
  Misses         2123     2123           

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update f53f67b...53d6fce. Read the comment docs.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

Im not sure we should have the "reset" argument in the future. It makes things unpredictable. But that is not stopping us from adding these tests.

$generator->addProvider(new class(...$words) {
private $words;

public function __construct(string ...$words)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the array deconstruction and not just pass the word array?

Copy link
Member Author

@localheinz localheinz Dec 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bramceulemans

Because when passing an array, it would be necessary to write more code to ensure that every element of the array is a string:

/**
 * @param array<int, string> $words
 */
public function __construct(array $words)
{
    $this->words = array_values(array_map(static function (string $word): string {
        return $word;
    }, $words));
}

or

/**
 * @param array<int, string> $words
 * 
 * @throws \InvalidArgumentException
 */
public function __construct(array $words)
{
    $notString = array_filter(static function ($word): bool {
        return !is_string($word);
    }, $words);

    if ([] !== $notString) {
        throw new \InvalidArgumentException('Every word needs to be a string');
    }

    $this->words = array_values($words);
}

Using variadics here allows me not to write this code.

@localheinz localheinz self-assigned this Dec 26, 2020
@localheinz localheinz merged commit 3d69b4d into FakerPHP:main Dec 26, 2020
@localheinz
Copy link
Member Author

Thank you, @Nyholm!

@localheinz localheinz deleted the fix/test branch December 26, 2020 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants